Codecov Report

Merging #1042 (cf2ea2e) into main (d2f5935) will decrease coverage by 0.21%.
The diff coverage is 65.61%.

❗ Current head cf2ea2e differs from pull request most recent head 3404df7. Consider uploading reports for the commit 3404df7 to get more accurate results

@@            Coverage Diff             @@
##             main    #1042      +/-   ##
==========================================
- Coverage   81.31%   81.09%   -0.22%     
==========================================
  Files         365      363       -2     
  Lines       34902    34723     -179     
==========================================
- Hits        28380    28160     -220     
- Misses       6522     6563      +41     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2f5935...3404df7. Read the comment docs.

@houqp, @Dandandan, @dbr, @sundy-li would you be available to review / comment / help on this one? We are trying to significantly improve the clone-on-write story, which is a bottleneck in Arrow, but would like to double-check with you that this does no harm.

may be of interest to @wesm also - I recall that Voltron was looking around avoiding allocations on compute kernels - the main point is (in C++ notation): since data is already inside a shared_ptr inside ArrayData, we can use unique_ptr<Array> instead of shared_ptr<Array> - cloning the Array is cheap anyways. The advantage is that we can easily access the array's content and thus can mutate the array itself in-place.

In cases where data is not shared (which is often, e.g. coming from parquet, IPC, etc.), this has dramatic effects since it can lead to a allocation-free compute for math.

E.g. expressions like exp(-(arr + 1)) * 2 + 1 (5 ops) can be done without allocations, still leverage SIMD, and do not require emitting new LLVM via e.g. Grandiva.

#1061 motivates this PR (~2x speedup on horizontal ops, ~5-10x on horizontal logic ops)

This change sounds perfectly fine from our point of view - we mostly interact with arrow2 data either in read-only fashion or via FFI (and the changes to FFI seem pretty trivial to handle) - so this doesn't really impact us directly, and if it's faster and generally better, then, 🥳

One thing I don't quite understand is,

Thus, Arcing Array [...] makes it challenging to offer a good support for clone-on-write semantics to arrays since we need to check if the array is being shared, and clone its internals if yes

I thought Arc provides clone-on-write? Specifically Arc::make_mut(...) docs say "If there are other Arc pointers to the same allocation, then make_mut will clone the inner value to a new allocation to ensure unique ownership. This is also referred to as clone-on-write"

@dbr, great question.

Let's consider the example added on this PR with an Arc instead of a Box:

// This example demos how to operate on arrays in-place.
use std::sync::Arc;
use arrow2::array::{Array, PrimitiveArray};

// say we have have received an array
let mut array: Arc<dyn Array> = PrimitiveArray::from_vec(vec![1i32, 2]).arced();

// we can apply a transformation to its values without allocating a new array as follows:
// 1. downcast it to the correct type (known via `array.data_type().to_physical_type()`)
let array = Arc::make_mut(&mut array)
    .as_any_mut()
    .downcast_mut::<PrimitiveArray<i32>>()
    .unwrap();

// 2. call `apply_values` with the function to apply over the values
array.apply_values(|x| x.iter_mut().for_each(|x| *x *= 10));

This unfortunately does not compile because Arc::<T>::make_mut requires T: Clone, but dyn Array is not clonable (since it is not Size). The crate dyn_clone offers a safe implementation of Clone for Box<dyn T> when T: Clone. We could get away with something like

// say we have have received an array
let array: Arc<dyn Array> = PrimitiveArray::from_vec(vec![1i32, 2]).arced();

// we can apply a transformation to its values without allocating a new array as follows:
// 1. clone the internals
let mut array: Box<dyn Array> = arrow2::array::clone(array.as_ref());

// 2. downcast it to the correct type (known via `array.data_type().to_physical_type()`)
let array = array
    .as_any_mut()
    .downcast_mut::<PrimitiveArray<i32>>()
    .unwrap();

// 3. call `apply_values` with the function to apply over the values
array.apply_values(|x| x.iter_mut().for_each(|x| *x *= 10));

but here we need to be careful - if we would have named the variable array1 instead of array, in step 3. both arrays are in scope and share the underlying data (due to clone). Thus, apply_values would result in a clone of O(N), since the values are now shared between the two arrays.

Looks great to me. cc @andylokandy @leiysky @b41sh may be interested in it if databend can introduce this into new expression framework.

Silly question about this change. I understood that the use of Arc in arrow was to have a Sync and Send array. Will Box provide the same functionality?

Forget this question, as soon as I finished writing I checked doc.rs and I see that Box does implement Sync and Send

Not sure if changelog is generated at version release time, but the only change I needed to make for my code that relied on chunks was to change Arc -> Box and it worked. 🚀